termios: Add TIOCSHALFD for ioctl() for enabling half-duplex uart#461
termios: Add TIOCSHALFD for ioctl() for enabling half-duplex uart#461olaf-czerwinski-phoenix-systems wants to merge 1 commit intomasterfrom
Conversation
Summary of ChangesHello @olaf-czerwinski-phoenix-systems, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new ioctl command, TIOCSHALFD, to enable half-duplex mode for UART. The implementation is straightforward and adds needed functionality. I have one suggestion regarding the API design to improve consistency with existing ioctl commands in the codebase by considering the addition of a corresponding 'get' command and adjusting the command numbering accordingly.
include/termios.h
Outdated
| #define TIOCSWINSZ _IOW('t', 0x14, struct winsize) | ||
| #define TIOCNOTTY _IO('t', 0x22) | ||
| #define TIOCGSID _IOR('t', 0x29, pid_t) | ||
| #define TIOCSHALFD _IOV('t', 0x2a, int) |
There was a problem hiding this comment.
For API consistency and to follow conventions in this file (e.g., TIOCGPGRP/TIOCSPGRP), it is good practice to provide a corresponding 'get' operation for a 'set' operation. The existing ioctl commands often follow a GET=N, SET=N+1 numbering scheme. To align with this pattern, I suggest using 0x2b for TIOCSHALFD and reserving 0x2a for a TIOCGHALFD. The 'get' command could be added now or in a follow-up PR and would likely look like this:
#define TIOCGHALFD _IOR('t', 0x2a, int)| #define TIOCSHALFD _IOV('t', 0x2a, int) | |
| #define TIOCSHALFD _IOV('t', 0x2b, int) |
There was a problem hiding this comment.
Kinda agree + perhaps move non-standard ones higher, e.g. 0x80 (so 0x80 + 0x81 combo)?
feba061 to
07ba8c4
Compare
agkaminski
left a comment
There was a problem hiding this comment.
LGTM. @nalajcie is this ok to add such custom ioctl? (Imho ok)
|
We'll probably have to add a custom ioctl, I don't think there are is anything standard that does exactly what is required here - the closest would be |
Yes, alternative approach I see is to add it to the stm32 specific header. But libtty has to be aware anyway, so the current solution is better |
nalajcie
left a comment
There was a problem hiding this comment.
- invalid commit message title (missing prefix)
- invalid 64bit impl (breaks riscv64 build)
- either invalid impl or declaration (impl takes
int**, notint) - implementation returns success even if half-duplex is not supported in uart driver
I'm ok with adding this functionality as a custom IOCTL(s), but please make the change sensible.
07ba8c4 to
5fe668c
Compare
@nalajcie I think that all issues should be addressed now. |
TASK: PP-428
Description
Add TIOCSHALFD for ioctl() for enabling half-duplex uart
Motivation and Context
Allows enabling half-duplex mode (for now supported in stm32n6 only).
Types of changes
How Has This Been Tested?
Checklist:
Special treatment